-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
minor: support @sentry/sveltekit routing instrumentation #98
Conversation
WalkthroughThe changes primarily focus on enhancing the Sentry SDK integration for Svelte applications. They introduce new functions to handle client integrations, apply SDK metadata, and manage the Changes
TipsChat with CodeRabbit Bot (
|
Deploying with Cloudflare Pages
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 4
Files ignored due to filter (2)
- package.json
- pnpm-lock.yaml
Files selected for processing (8)
- packages/lib/scripts/build.ts (1 hunks)
- packages/lib/src/client/addClientIntegrations.ts (1 hunks)
- packages/lib/src/client/applySdkMetadata.ts (1 hunks)
- packages/lib/src/client/init.ts (2 hunks)
- packages/lib/src/client/restoreFetch.ts (1 hunks)
- packages/lib/src/client/router.ts (1 hunks)
- packages/lib/src/client/sentryInit.ts (1 hunks)
- packages/lib/src/client/switchToFetchProxy.ts (1 hunks)
Files skipped from review due to trivial changes (3)
- packages/lib/scripts/build.ts
- packages/lib/src/client/init.ts
- packages/lib/src/client/router.ts
Additional comments (Suppressed): 1
packages/lib/src/client/restoreFetch.ts (1)
- 1-11: The
restoreFetch
function is replacing the globalfetch
function with the originalfetch
function passed as an argument. This is a good practice to restore the original behavior after the Sentry SDK initialization. However, it's not clear why thefetch
function is being stored in_sentryFetchProxy
on line 7. If this is not used elsewhere, it might be unnecessary. Please verify.- globalWithSentryFetchProxy._sentryFetchProxy = - globalWithSentryFetchProxy.fetch - globalWithSentryFetchProxy.fetch = actualFetch
import { hasTracingEnabled } from '@sentry/core' | ||
import { BrowserOptions, BrowserTracing } from '@sentry/svelte' | ||
import { addOrUpdateIntegration } from '@sentry/utils' | ||
import { svelteKitRoutingInstrumentation } from './router.js' | ||
|
||
export const addClientIntegrations = (options: BrowserOptions) => { | ||
let integrations = options.integrations || [] | ||
|
||
if (hasTracingEnabled(options)) { | ||
const defaultBrowserTracingIntegration = new BrowserTracing({ | ||
routingInstrumentation: svelteKitRoutingInstrumentation | ||
}) | ||
|
||
integrations = addOrUpdateIntegration( | ||
defaultBrowserTracingIntegration, | ||
integrations, | ||
{ | ||
'options.routingInstrumentation': svelteKitRoutingInstrumentation | ||
} | ||
) | ||
} | ||
|
||
options.integrations = integrations | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function addClientIntegrations
modifies the options
object passed to it. This can lead to unexpected side effects if the caller is not aware of this behavior. Consider returning a new object with the updated integrations instead of modifying the input object. This would make the function pure and easier to reason about.
- options.integrations = integrations
+ return { ...options, integrations };
Then, the caller can decide whether to replace the original options object or not.
import { WINDOW } from '@sentry/svelte' | ||
|
||
export function switchToFetchProxy() { | ||
const globalWithSentryFetchProxy = WINDOW | ||
|
||
const actualFetch = globalWithSentryFetchProxy.fetch | ||
|
||
// @ts-expect-error TODO: fix this | ||
if (globalWithSentryFetchProxy._sentryFetchProxy && actualFetch) { | ||
globalWithSentryFetchProxy.fetch = | ||
// @ts-expect-error TODO: fix this | ||
globalWithSentryFetchProxy._sentryFetchProxy | ||
return actualFetch | ||
} | ||
|
||
return undefined | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function switchToFetchProxy
switches the global fetch
function to a Sentry proxy. However, it doesn't check if the _sentryFetchProxy
is a function before assigning it to fetch
. This could potentially break the application if _sentryFetchProxy
is not a function. Consider adding a type check before the assignment.
- if (globalWithSentryFetchProxy._sentryFetchProxy && actualFetch) {
+ if (typeof globalWithSentryFetchProxy._sentryFetchProxy === 'function' && actualFetch) {
Also, the function returns undefined
if the conditions are not met. This could lead to unexpected behavior in the calling code. Consider throwing an error or returning a default value instead.
import { BrowserOptions, configureScope, init } from '@sentry/svelte' | ||
import { addClientIntegrations } from './addClientIntegrations.js' | ||
import { applySdkMetadata } from './applySdkMetadata.js' | ||
import { restoreFetch } from './restoreFetch.js' | ||
import { switchToFetchProxy } from './switchToFetchProxy.js' | ||
|
||
export const sentryInit = (options: BrowserOptions) => { | ||
applySdkMetadata(options, ['sveltekit', 'svelte']) | ||
|
||
addClientIntegrations(options) | ||
|
||
// 1. Switch window.fetch to our fetch proxy we injected earlier | ||
const actualFetch = switchToFetchProxy() | ||
|
||
// 2. Initialize the SDK which will instrument our proxy | ||
init(options) | ||
|
||
// 3. Restore the original fetch now that our proxy is instrumented | ||
if (actualFetch) { | ||
restoreFetch(actualFetch) | ||
} | ||
|
||
configureScope((scope) => { | ||
scope.setTag('runtime', 'browser') | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sentryInit
function is well-structured and follows a clear sequence of steps for initializing Sentry with SvelteKit. However, there is a potential issue with the restoreFetch
function. If switchToFetchProxy
fails and returns null
or undefined
, restoreFetch
will be called with an undefined argument. This could lead to unexpected behavior or errors. Consider adding error handling or a check to ensure actualFetch
is a function before restoring it.
// 3. Restore the original fetch now that our proxy is instrumented
if (actualFetch && typeof actualFetch === 'function') {
restoreFetch(actualFetch)
}
import { SDK_VERSION } from '@sentry/core' | ||
import { BrowserOptions } from '@sentry/svelte' | ||
|
||
const PACKAGE_NAME_PREFIX = 'npm:@sentry/' | ||
|
||
export const applySdkMetadata = (options: BrowserOptions, names: string[]) => { | ||
options._metadata = options._metadata || {} | ||
options._metadata.sdk = options._metadata.sdk || { | ||
name: 'sentry.javascript.sveltekit', | ||
packages: names.map((name) => ({ | ||
name: `${PACKAGE_NAME_PREFIX}${name}`, | ||
version: SDK_VERSION | ||
})), | ||
version: SDK_VERSION | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function applySdkMetadata
modifies the options
object passed to it. This is a side effect that could lead to unexpected behavior if the caller is not aware of it. Consider returning a new object instead of modifying the existing one. This would make the function pure and easier to reason about.
- export const applySdkMetadata = (options: BrowserOptions, names: string[]) => {
- options._metadata = options._metadata || {}
- options._metadata.sdk = options._metadata.sdk || {
- name: 'sentry.javascript.sveltekit',
- packages: names.map((name) => ({
- name: `${PACKAGE_NAME_PREFIX}${name}`,
- version: SDK_VERSION
- })),
- version: SDK_VERSION
- }
- }
+ export const applySdkMetadata = (options: BrowserOptions, names: string[]): BrowserOptions => {
+ const newOptions = { ...options };
+ newOptions._metadata = newOptions._metadata || {}
+ newOptions._metadata.sdk = newOptions._metadata.sdk || {
+ name: 'sentry.javascript.sveltekit',
+ packages: names.map((name) => ({
+ name: `${PACKAGE_NAME_PREFIX}${name}`,
+ version: SDK_VERSION
+ })),
+ version: SDK_VERSION
+ }
+ return newOptions;
+ }
close #97
Summary by CodeRabbit
$app/stores
as an external dependency, improving the overall build process.BrowserOptions
object with new functions for adding client integrations and applying SDK metadata. This will provide more customization options and better tracking of SDK usage.fetch
function to a proxy, improving the handling of network requests.Please note, there are a couple of TypeScript errors that need to be addressed in the new
switchToFetchProxy
function.